Test build #23304 has started for PR 3241 at commit 409f3af.

• This patch merges cleanly.

Yes, throwing Exception or RuntimeException is bad practice in Java. That said, I do think you're right that it changes the signature in the bytecode, since this is a checked exception. I don't think it's worth that. At the least, rather than change this once, change it everywhere and wait until a breaking change is acceptable.

Test build #23304 has finished for PR 3241 at commit 409f3af.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23304/
Test PASSed.

Does this definitely change the bytecode signature? If so, I wonder why we don't get a compatiblity warning.

We're throwing a more specific exception class, so I guess that caller code doesn't need to change: if the old caller code caught Exception then it can certainly catch SparkException since that's a subclass. Things would be different if we changed this to throw a wider exception type.

I think this compatibility concern is moot, though, since checkpoint() wasn't declared with the @throws Scala annotation, so the generated code method won't have any declared exceptions:

scala> import java.io.IOException
import java.io.IOException

scala> class A {
     |   def foo() {
     |    throw new IOException("error!")
     |   }
     | }
defined class A

scala> :javap -s A
Compiled from "<console>"
public class A {
  public void foo();
    Signature: ()V

  public A();
    Signature: ()V
}

scala> class B {
     |   @throws[IOException]
     |   def foo() {
     |      throw new IOException("error!")
     |   }
     | }
defined class B

scala> :javap -s B
Compiled from "<console>"
public class B {
  public void foo() throws java.io.IOException;
    Signature: ()V

  public B();
    Signature: ()V
}

Hm, I may have learned something today. I don't think it is even in the signature of the method when scalac compiles it. See http://www.scala-lang.org/old/node/106 or http://engineering.richrelevance.com/exceptional-exceptions/#advanced Sure enough javap says that the exception attributes aren't in the bytecode. So I don't think this does change the signature after all.

Does that mean if we went the other way (i.e. changing it from SparkException to Exception) it would be bytecode incompatible?

Exception won't be in the method signature.

scala> import java.io.IOException
import java.io.IOException

scala> class A {
     |   @throws[IOException]
     |   def foo() {
     |     throw new IOException("error!")
     |   }
     | 
     |   def bar(): Unit = {
     |     foo()
     |   }
     | }
defined class A

scala> :javap -private -c A
Compiled from "<console>"
public class A extends java.lang.Object{
public void foo()   throws java.io.IOException;
  Code:
   0:   new #9; //class java/io/IOException
   3:   dup
   4:   ldc #11; //String error!
   6:   invokespecial   #15; //Method java/io/IOException."<init>":(Ljava/lang/String;)V
   9:   athrow

public void bar();
  Code:
   0:   aload_0
   1:   invokevirtual   #20; //Method foo:()V
   4:   return

public A();
  Code:
   0:   aload_0
   1:   invokespecial   #22; //Method java/lang/Object."<init>":()V
   4:   return

}

In the bar(), the instruction is 1: invokevirtual #20; //Method foo:()V. The method is still foo:()V.

I'm sorry that I should have been clearer when I said the breaking change. I'm worried about the following case:

  try {
    rdd.checkpoint()
  } catch {
    case e: SparkException => // do work A
    case e: Exception => do work B
  }

It breaks such case. However, I think few people will write such code.

Therefore, does Spark view such change as a breaking change?

I think this should be ok. Because we didn't really document the exception, it is not strictly part of the contract.

I'm merging this in master.